-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Refactor keepalive-related fields in confighttp #14203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Refactor keepalive-related fields in confighttp #14203
Conversation
@codeboten Is this fear unfounded? Searching I could find open-telemetry/opentelemetry-specification/issues/4440 which does mention keepalives explicitly, but I don't know how to think about this |
CodSpeed Performance ReportMerging #14203 will degrade performances by 43.39%Comparing
|
| Benchmark | BASE |
HEAD |
Change | |
|---|---|---|---|---|
| ❌ | zstdNoConcurrency |
29.5 µs | 52.2 µs | -43.39% |
| ❌ | BenchmarkSplittingBasedOnItemCountManySmallLogs |
2.2 ms | 3.2 ms | -32.63% |
| ❌ | BenchmarkLogsMarshalJSON |
2.8 µs | 3.9 µs | -27.95% |
| ❌ | BenchmarkTraceSizeBytes |
322.9 µs | 431.6 µs | -25.19% |
|
I'm supportive of this approach. I think we can finalize the migration before marking confighttp 1.0 |
Since the OTLP exporter and the OTLP receiver are marked as 1.0 I think we would not be able to complete this migration: we can deprecate the older options but we need to keep compatibility with them until a 2.0 version of these components. |
Description
This draft PR shows how we could reorganize legacy fields.
I am not convinced that we should do this because it changes quite a few options and it can be disruptive to end users, specially given that, because of the declarative configuration, this may go in a different direction upstream and we want to stay aligned if possible.
Link to tracking issue
Fixes #14020